Skip to content

Conversation

@marisn
Copy link
Contributor

@marisn marisn commented May 28, 2025

An attempt to unify checks and error messages.

There are more places where lseek return value should be checked, but I don't have time to work on them now.

An attempt to unify checks and error messages
@marisn marisn added the C Related code is in C label May 28, 2025
@github-actions github-actions bot added raster Related to raster data processing libraries module imagery raster3d labels May 28, 2025
@marisn marisn requested a review from petrasovaa May 28, 2025 02:37
@wenzeslaus
Copy link
Member

Perhaps this already revealed some errors. It fails tests:

Running ./raster/r.random/testsuite/test_r_random.py...
========================================================================
FFFFFFFF
======================================================================
FAIL: test_random_raster (__main__.TestRasterTile)
Testing r.random  runs successfully
----------------------------------------------------------------------
Traceback (most recent call last):
  File "etc/python/grass/gunittest/case.py", line 1396, in assertModule
    module.run()
  File "etc/python/grass/pygrass/modules/interface/module.py", line 825, in run
    self.wait()
  File "etc/python/grass/pygrass/modules/interface/module.py", line 846, in wait
    raise CalledModuleError(
grass.exceptions.CalledModuleError: Module run `r.random input=landcover_1m npoints=20 seed=1 raster=landcover_1m_raster_random` ended with an error.
The subprocess ended with a non-zero return code: 1. See the following errors:
b'Collecting Stats...\n0..3..6..9..12..15..18..21..24..27..30..33..36..39..42..45..48..51..54..57..60..63..66..69..72..75..78..81..84..87..90..93..96..99..100\nERROR: Unable to seek: 29 Illegal seek\n'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "raster/r.random/testsuite/test_r_random.py", line 59, in test_random_raster
    self.assertModule(
  File "etc/python/grass/gunittest/case.py", line 1416, in assertModule
    self.fail(self._formatMessage(msg, stdmsg))
AssertionError: Running <r.random> module ended with non-zero return code (1)
Called (Python): r.random(input='landcover_1m', npoints='20', raster='landcover_1m_raster_random', seed=1)
Called (Bash): r.random input=landcover_1m npoints=20 seed=1 raster=landcover_1m_raster_random
See the following errors:
Collecting Stats...
0..3..6..9..12..15..18..21..24..27..30..33..36..39..42..45..48..51..54..57..60..63..66..69..72..75..78..81..84..87..90..93..96..99..100
ERROR: Unable to seek: 29 Illegal seek


======================================================================

@marisn
Copy link
Contributor Author

marisn commented May 29, 2025

Perhaps this already revealed some errors. It fails tests:

Yes, as it just checks if a return value does not indicate on an error, this really is a bug in r.random. If I read the code correctly, there is no need to call lseek in r.random at all. I'll look into this tomorrow.

Rewinding a raster map can lead to lseek errors and generally
should not be done, as raster reading functions perform seek
on their own if required.
@marisn
Copy link
Contributor Author

marisn commented May 29, 2025

MacOS failure is caused by #5787

@echoix
Copy link
Member

echoix commented May 30, 2025

Concerning the message that you applied almost everywhere, isn't it similar to one of your examples last week where %d %s can't be reordered when requesting translations? (And they are collected for translation)

@nilason
Copy link
Contributor

nilason commented Jun 1, 2025

There shouldn’t be any reason to cast -1 to off_t when directly comparing against the results of lseek. Not a big deal, but just adds clutter (in my opinion).

@marisn marisn marked this pull request as draft June 9, 2025 07:59
@marisn
Copy link
Contributor Author

marisn commented Jul 19, 2025

There shouldn’t be any reason to cast -1 to off_t when directly comparing against the results of lseek. Not a big deal, but just adds clutter (in my opinion).

That is if we don't want to be K&R compatible any more ;-)

* no need to cast -1 to off_t as we do not use K&R C
* make error message arguments relocable
* unify error messges
* add missing includes for error reporting
@github-actions github-actions bot added the vector Related to vector data processing label Jul 19, 2025
@marisn
Copy link
Contributor Author

marisn commented Jul 19, 2025

Concerning the message that you applied almost everywhere, isn't it similar to one of your examples last week where %d %s can't be reordered when requesting translations? (And they are collected for translation)

Apparently %n$ is a POSIX extension to C. It is supported by all POSIX compilers, including MSVC, but will fail to work in strict ISO C mode. Thus either we abandon strict ISO C requirement or %n$ can not be used.

@echoix
Copy link
Member

echoix commented Jul 19, 2025

Concerning the message that you applied almost everywhere, isn't it similar to one of your examples last week where %d %s can't be reordered when requesting translations? (And they are collected for translation)

Apparently %n$ is a POSIX extension to C. It is supported by all POSIX compilers, including MSVC, but will fail to work in strict ISO C mode. Thus either we abandon strict ISO C requirement or %n$ can not be used.

I think I read since that comment, that they support positional placeholders, but through a different function name that supports positional explicitly (or maybe even exclusively). I don't think there's this distinction in other platforms

@wenzeslaus
Copy link
Member

What do you think about the message? I think that everyone needs that comment about what this refers to, not only the translators.

@marisn
Copy link
Contributor Author

marisn commented Jul 20, 2025

I think I read since that comment, that they support positional placeholders, but through a different function name that supports positional explicitly (or maybe even exclusively). I don't think there's this distinction in other platforms

If you refer here to MS C compiler, then yes – we will have to IFDEF code in G_message et al.

From the documentation: „By default, if no positional formatting is present, the positional functions behave identically to the non-positional ones.“ Thus there would be no problems to replace *printf with _*printf_p in messaging functions.

@marisn
Copy link
Contributor Author

marisn commented Jul 20, 2025

What do you think about the message? I think that everyone needs that comment about what this refers to, not only the translators.

I am not good at this. Like „File seek error with code %d: %s“?

@nilason
Copy link
Contributor

nilason commented Jul 21, 2025

What do you think about the message? I think that everyone needs that comment about what this refers to, not only the translators.

I am not good at this. Like „File seek error with code %d: %s“?

What about: "File seek error: %s (%d)", which would have resulted in e.g. "ERROR: File seek error: Illegal seek (29)"?

@nilason
Copy link
Contributor

nilason commented Jul 21, 2025

What do you think about the message? I think that everyone needs that comment about what this refers to, not only the translators.

I am not good at this. Like „File seek error with code %d: %s“?

What about: "File seek error: %s (%d)", which would have resulted in e.g. "ERROR: File seek error: Illegal seek (29)"?

Or skip the ambivalent word "seek" with e.g. "File read/write operation failed: %s (%d)", giving "ERROR: File read/write operation failed: Illegal seek (29)".

@wenzeslaus
Copy link
Member

Replacing the word "seek" is the way go. More specificity about what file and where would be nice, but as a general and quick way how to implement this, the last suggestion is good.

@marisn marisn marked this pull request as ready for review September 29, 2025 14:57
@marisn
Copy link
Contributor Author

marisn commented Sep 29, 2025

Replacing the word "seek" is the way go. More specificity about what file and where would be nice, but as a general and quick way how to implement this, the last suggestion is good.

Feel free to write a custom error message for each case after I merge this PR :P

@echoix
Copy link
Member

echoix commented Oct 22, 2025

Is there anything else appart reading and resolving the last comments remaining to do in this PR?

@marisn
Copy link
Contributor Author

marisn commented Oct 25, 2025

Is there anything else appart reading and resolving the last comments remaining to do in this PR?

I don't think so. Code cleaning and improving is a never ending story, let's get this in (and off my neck).

Copy link
Member

@echoix echoix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving you the chance to merge if it works fine, I might reread again later

@echoix
Copy link
Member

echoix commented Oct 25, 2025

I'll launch manually this week's updating of translations right after having this merged, to not have it wait another week

@echoix echoix merged commit ee87f21 into OSGeo:main Oct 25, 2025
27 checks passed
@github-actions github-actions bot added this to the 8.5.0 milestone Oct 25, 2025
@marisn marisn deleted the r.smooth.lseek branch October 26, 2025 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C Related code is in C imagery libraries module raster Related to raster data processing raster3d vector Related to vector data processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants